bug: use better host validation for sub
#92
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While far from perfect, this is a little better about checking the sorts of values passed in the `sub` claim. It checks for ipv6 like as well as localhost in mailto and https forms. (Yeah, you'll still need to supply `https` since that's part of the RFC. Plus, letsEncrypt is a thing, so there's that.) I've also introduced a new option `--no-strict` which turns off `sub` checks. It has to be present, but what the value is can be up to you. I'll note that this kinda ruins what VAPID is supposed to be about. The `sub` is there so that if there's a problem with your subscription, Ops can reach out to you to help fix it rather than just straight up block you. Closes: #90
pjenvey
approved these changes
Mar 29, 2021
| @@ -247,3 +252,29 @@ def test_invalid_sig(self, mm): | |||
| decode, | |||
| 'foo.bar.a', | |||
| 'aaaa') | |||
|
|
|||
| def test_sub(self): | |||
There was a problem hiding this comment.
Rarely do I use py.test's parameterized tests (it's a bit of magic) but I may have actually considered it here
There was a problem hiding this comment.
Maybe? I dunno. For something like this, I think it's clearer for future me to be less clever and just specify what goes into which gauntlet as a delineated list like this, rather than as a CSV.
There was a problem hiding this comment.
Yea that's part of why I don't use it, it's a bit too clever. I think the main benefit is debugging and test output is a bit improved over a loop
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While far from perfect, this is a little better about checking the sorts
of values passed in the
subclaim. It checks for ipv6 like as well aslocalhost in mailto and https forms. (Yeah, you'll still need to supply
httpssince that's part of the RFC. Plus, letsEncrypt is a thing, sothere's that.)
I've also introduced a new option
--no-strictwhich turns offsubchecks. It has to be present, but what the value is can be up toyou. I'll note that this kinda ruins what VAPID is supposed to be about.
The
subis there so that if there's a problem with your subscription,Ops can reach out to you to help fix it rather than just straight up
block you.
Closes: #90